-
Notifications
You must be signed in to change notification settings - Fork 819
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Mutablebuffer::shrink_to_fit #318
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
Two comments:
- Could we add a test? ^_^
- I am not sure we need
inline
this; inlining is useful when growing because it is used over an iterator withpush
; I am not sure it is needed for shrinking
Definitely! It was already in the docstring, but I will add an explicit one.
I agree. 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM -- thanks @ritchie46
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great 👍
Codecov Report
@@ Coverage Diff @@
## master #318 +/- ##
=======================================
Coverage 82.52% 82.52%
=======================================
Files 162 162
Lines 44007 44021 +14
=======================================
+ Hits 36316 36329 +13
- Misses 7691 7692 +1
Continue to review full report at Codecov.
|
* Mutablebuffer::shrink_to_fit * add shrink_to_fit explicit test
* Mutablebuffer::shrink_to_fit * add shrink_to_fit explicit test
I am wondering if we should do Any thoughts? |
I discussed this idea with @jorgecarleitao. But it turns out that if we call |
I was looking for some documentation about whether Did we have any evidence or tests for what, in general, allocators are doing here? @jorgecarleitao |
Yes, you are rigtht. A quick google documentation survey: malloc is not really clear about how it mi-malloc says it will shrink in place: jemalloc says it will shrink in place: |
Something to consider. I think that it may lead to increased fragmentation. If we allocate |
Would that be a problem over not shrinking at all? If you don't shrink, the (smaller amount of) remaining memory might be more fragmented than the chunk that can be removed with shrinking. |
Which issue does this PR close?
This PR add
shrink_to_fit
to the public API ofMutableBuffer
. This closes #297.In line of this, I'd like the option to shrink existing
array
data, where would such logic be a best fit. I was thinking membership ofdyn Array
trait.